fix(sep-2243): reject x-mcp-header on number-typed params - #444
Conversation
SEP-2243 permits `x-mcp-header` only on integer, string and boolean parameters — "Parameters with type `number` are not permitted" — but the harness never verified the `number` case and its requirement text still described `number` as permitted. PR modelcontextprotocol#371 fixed the positive `http-custom-headers` fixture (priority is now `integer`, `float_val` is served unannotated) but left the two remaining items from modelcontextprotocol#344: - `HttpInvalidToolHeadersScenario` serves invalid tools for every other x-mcp-header constraint but had no number-typed case, so nothing checked that a client excludes such a tool. - `sep-2243.yaml` claimed primitive types are "(number, string, or boolean)" and that "number values" get decimal-string encoding, both of which contradict the released 2026-07-28 spec. Add an `invalid_number_header` tool mapped to the existing `sep-2243-x-mcp-header-primitive-only` requirement (no new check IDs) and correct both requirement texts. traceability.json is updated in place since it embeds the yaml text verbatim; check IDs and statuses are unchanged. Verified against typescript-sdk@main: the new check fails there because the SDK deliberately allow-lists `number` to satisfy the old fixture — a workaround its own source notes as "tracked upstream" and can now drop. The positive scenario still passes 18/18. Closes modelcontextprotocol#344 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
commit: |
JosephDoUrden
left a comment
JosephDoUrden
left a comment
There was a problem hiding this comment.
Ran the whole thing locally. 505/505 on the PR head, which matches the body. Then merged current main in (post #447) to be sure, still a clean merge, 525/525. I also deleted the invalid_number_header mapping line to check the new test is load-bearing, it fails with exactly the assertion error quoted in the body, so it is.
Both yaml text rewrites check out quote-level against the released 2026-07-28 spec, and the hand-edited traceability block matches a fresh computeTraceability recompute byte for byte. The ts-sdk discrepancy claim is real too, PERMITTED_X_MCP_HEADER_TYPES on main still includes 'number'.
One follow-up worth a note, not blocking and not caused here: the manifest has some pre-existing staleness. There is no 1932 block at all, and two sep-2243 server rows still say -32001 where both the yaml and the released spec say -32020. A regeneration against a reference run would clear both.
Looks merge-ready to me.
2 participants
Closes #344
Why this is still open after #371
SEP-2243 permits
x-mcp-headeronly on integer, string and boolean parameters. The released2026-07-28spec is explicit:and the value-encoding table in
basic/transports/streamable-http.mdxlists onlystring/integer/boolean.#371 fixed the positive fixture (
priorityis nowinteger,float_valis served unannotated and asserted not mirrored) but left the two items #344 listed as optional, which is why the issue stayed open. This finishes them.Changes
1. Negative case for the
numberrule.HttpInvalidToolHeadersScenarioserved invalid tools for every otherx-mcp-headerconstraint — empty / object / array / null / duplicate / space / colon / non-ascii / control-char — but had no number-typed one.sep-2243-x-mcp-header-primitive-onlywas only ever exercised via object/array/null, so nothing verified the rule the issue was actually about. Adds aninvalid_number_headertool withscore: { type: 'number', 'x-mcp-header': 'Score' }, mapped to the existing requirement.No check IDs added or removed —
INVALID_TOOL_DECLARED_CHECK_IDSis unchanged.2. Requirement text corrected.
sep-2243.yamlstill described the permitted set as "(number, string, or boolean)" and said "number values MUST be converted to their decimal string representation" — both contradict the released spec.traceability.jsonembeds this text verbatim, so its twotextfields are updated in place; check IDs andstatusare untouched.Verification
npm run typecheck,npm run lint,npm run buildclean;npm test505 passed / 43 files.INVALID_TOOL_CONSTRAINT_IDSmapping makes the new test fail (expected [ 'SUCCESS', 'SUCCESS', 'SUCCESS' ] to include 'FAILURE'), so it isn't vacuous.http-custom-headersstill 18/18.The new check finds a real SDK bug
http-invalid-tool-headersagainst typescript-sdk@main goes 11/12 —sep-2243-x-mcp-header-primitive-onlyfails oninvalid_number_header. The cause is in the SDK, not the harness:numberwas allow-listed there specifically to tolerate this fixture bug, with a comment noting the discrepancy is "tracked upstream." With #371 having fixed the fixture, that workaround can be dropped. Worth a follow-up on the SDK side.This won't red-light CI:
ci.ymlruns onlynpm ci/check/build/test. The typescript-sdk run lives intraceability.yml, which is manual/scheduled, uses|| true, and per AGENTS.md is not a PR gate.Out of scope
#354 (positive scenario's
ttlMs: 0vs. SEP-2549 stale-schema) is a separate defect owned by #358. The positive scenario's TTL is untouched here, and #358's scope note confirms it doesn't modify the invalid-tool scenario — so these shouldn't collide beyond a trivial rebase.